Unify streamAdapter/betaStreamAdapter retry logic into generic retryableStream#2018
Merged
dgageot merged 1 commit intodocker:mainfrom Mar 10, 2026
Merged
Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Assessment: 🟡 NEEDS ATTENTION
This PR successfully consolidates duplicated retry logic into a generic retryableStream[T] structure, reducing code duplication. However, there is one confirmed resource leak issue in the retry path that should be addressed.
Confirmed Issue
MEDIUM: Stream leak in retry failure path (retry.go:35)
When the retry succeeds and r.stream is replaced with newStream, if the subsequent recursive r.next() call fails, the function returns the error without closing the newly assigned stream. This creates a resource leak.
Review Notes
The refactoring is well-structured:
- Generic
retryableStream[T]properly encapsulates retry logic - Adapter state (callbacks, flags) correctly persists across retry
- The
getResponseTrailercallback automatically accesses the latest HTTP response - Stream initialization guarantees make the removed nil checks safe
rumpl
previously approved these changes
Mar 10, 2026
…bleStream Extract the duplicated single-retry-on-context-length-error logic from both streamAdapter and betaStreamAdapter into a shared generic retryableStream[T] struct. Both adapters now embed retryableStream and use its next() method instead of duplicating the retry code inline. The retryFn closures in client.go and beta_client.go now return the raw *ssestream.Stream[T] directly instead of wrapping it in a full adapter, since only the stream was ever used from the returned value. Assisted-By: docker-agent
dgageot
force-pushed
the
board/unify-retry-logic-d5b3914d
branch
from
c18c55d to
fbf4e10
Compare
rumpl
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extract the duplicated single-retry-on-context-length-error logic from both
streamAdapterandbetaStreamAdapterinto a shared genericretryableStream[T]struct. Both adapters now embed
retryableStreamand use itsnext()methodinstead of duplicating the retry code inline.
The
retryFnclosures inclient.goandbeta_client.gonow return the raw*ssestream.Stream[T]directly instead of wrapping it in a full adapter,since only the stream was ever used from the returned value.
Changes
retry.go— GenericretryableStream[T]with the shared retry-on-context-length-error logic.adapter.go—streamAdapterembedsretryableStream; removed duplicated retry code.beta_adapter.go—betaStreamAdapterembedsretryableStream; removed duplicated retry code.client.go/beta_client.go—retryFnclosures return raw*ssestream.Stream[T]instead of full adapters.